Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance spo file roleassignment command #6431

Closed
wants to merge 2 commits into from
Closed

Conversation

ktskumar
Copy link
Contributor

Closes #6197
Added two options entratGroupId and entraGroupName to below commands
spo file roleassignement add
spo file roleassignement remove

@milanholemans
Copy link
Contributor

Thanks, we'll review it ASAP!

@Adam-it Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 29, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 29, 2024

@ktskumar I added the hacktoberfest-accepted label to this PR which means that this PR will count as done for the Hacktoberfest event. So if you participate in this event it will get you unblocked and it allows us to merge this PR later when we catch up 👍
Thanks for your support and awesome contribution 👏 You Rock 🤩

@Adam-it Adam-it self-assigned this Nov 20, 2024
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktskumar awesome work so far 👏👏👏👏
Let's do a bit of a clean up before we merge 🚀. You Rock 🤩

docs/docs/cmd/spo/file/file-roleassignment-add.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-add.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-add.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-add.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-add.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spo/file/file-roleassignment-remove.mdx Outdated Show resolved Hide resolved
src/m365/spo/commands/file/file-roleassignment-remove.ts Outdated Show resolved Hide resolved
@Adam-it Adam-it marked this pull request as draft November 20, 2024 23:27
@Adam-it
Copy link
Member

Adam-it commented Nov 26, 2024

@pnp/cli-for-microsoft-365-maintainers due to my short brake I am unassigning myself from this one so that it may be processed by someone else

@Adam-it Adam-it removed their assignment Nov 26, 2024
@ktskumar ktskumar marked this pull request as ready for review December 21, 2024 08:15
@Adam-it Adam-it self-assigned this Jan 3, 2025
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and works nicely as well.
I added a few small comments I noticed that I will resolve myself when merging.
Awesome work 👏

: The group name of the SharePoint group Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple. This option is only used for SharePoint Online groups.

`--entraGroupId [entraGroupId]`
: ID of the Microsoft Entra group to add. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command is about removing role assignment

Suggested change
: ID of the Microsoft Entra group to add. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.
: ID of the Microsoft Entra group to remove. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.

: ID of the Microsoft Entra group to add. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.

`--entraGroupName [entraGroupName]`
: Display name of the Microsoft Entra group to add. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be remove

Suggested change
: Display name of the Microsoft Entra group to add. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.
: Display name of the Microsoft Entra group to remove. Specify either `upn`, `groupName`, `entraGroupId`, `entraGroupName`, or `principalId` but not multiple.

Comment on lines +68 to +70
```sh
m365 spo file roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --fileId "b2307a39-e878-458b-bc90-03bc578531d6" --entraGroupId 3c97e01e-978d-417b-a196-b6a3e37fa873 --force

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we are missing the closing of code block

Suggested change
```sh
m365 spo file roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --fileId "b2307a39-e878-458b-bc90-03bc578531d6" --entraGroupId 3c97e01e-978d-417b-a196-b6a3e37fa873 --force
```sh
m365 spo file roleassignment remove --webUrl "https://contoso.sharepoint.com/sites/contoso-sales" --fileId "b2307a39-e878-458b-bc90-03bc578531d6" --entraGroupId 3c97e01e-978d-417b-a196-b6a3e37fa873 --force

@Adam-it
Copy link
Member

Adam-it commented Jan 4, 2025

Ready to merge 🚀. I should perform a small fixup when merging

@Adam-it
Copy link
Member

Adam-it commented Jan 8, 2025

Merged manually. Thank you for your awesome contribution. You Rock 🤩👏

@Adam-it Adam-it closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later pr-merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance spo file roleassignment commands with Microsoft Entra groups
3 participants